Scripting: body access decoded and array jsonpath in exchange expressions#2236
Conversation
…th accept arrays as input.
…th accept arrays as input.
WalkthroughRenames ExchangeExpression.newInstance(...) to ExchangeExpression.expression(...) across the codebase, relocates SpELBody to lang.LazyBody with API updates, adjusts CallInterceptor header order and copies decoded body, adds body binding for scripting, updates JsonPath deserialization, and renames test helper CHOICE→CHOOSE. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExchangeExpression
participant Concrete as ExprImpl
Note over Caller,ExchangeExpression: Expression creation (call-site change only)
Caller->>ExchangeExpression: expression(router, language, expr)
ExchangeExpression->>Concrete: switch(language) -> new ExprImpl(...)
Concrete-->>Caller: ExchangeExpression instance
sequenceDiagram
participant Client
participant CallInterceptor
participant Backend
Client->>CallInterceptor: incoming exchange
CallInterceptor->>Backend: call backend
Backend-->>CallInterceptor: response (encoded stream)
Note right of CallInterceptor: use getBodyAsStreamDecoded() to read body
CallInterceptor->>CallInterceptor: exc.getRequest().setBodyContent(decodedBytes)
CallInterceptor->>CallInterceptor: remove headers [SERVER, CONTENT_ENCODING, TRANSFER_ENCODING]
CallInterceptor-->>Client: forward modified request/response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/CallInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELBody.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/flow/ChooseInterceptorTest.java(3 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/flow/invocation/FlowTestInterceptors.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetPropertyInterceptorSpELTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyKeyComplexMatchTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (21)
core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELBody.java (1)
33-36: LGTM! Clean string representation for SpEL interpolation.The
toString()override provides decoded body access for SpEL expressions, aligning with the PR's objective to enhance body handling in scripting contexts.core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (2)
74-74: LGTM! Useful addition for scripting contexts.Exposing the decoded body string as a parameter makes it directly accessible in scripts, complementing the existing parsed "json" parameter for different use cases.
87-93: LGTM! Improved documentation format.Reformatting to Javadoc style improves readability and follows standard Java documentation conventions.
core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (1)
28-28: LGTM! Clean refactoring to static factory method.The migration from
newInstance()toexpression()with a static import improves code readability and aligns with the PR-wide refactoring pattern.Also applies to: 40-40
core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
29-29: LGTM! Consistent factory method migration.The change from
newInstance()toexpression()maintains the same behavior while improving consistency across the codebase.Also applies to: 56-56
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (1)
38-38: LGTM! Aligned with codebase-wide refactoring.The static factory method adoption is consistent with changes across other components, maintaining behavior while improving code clarity.
Also applies to: 86-86
core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java (1)
32-32: LGTM! Factory method refactoring applied correctly.The change follows the established pattern across the PR, replacing
newInstance()with the staticexpression()factory method.Also applies to: 60-60
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
28-28: LGTM! Consistent static factory method adoption.The migration to
expression()is properly implemented and aligns with the systematic refactoring across all expression-based components.Also applies to: 57-57
core/src/test/java/com/predic8/membrane/core/interceptor/flow/ChooseInterceptorTest.java (1)
29-29: LGTM! Naming consistency improvement.The rename from
CHOICEtoCHOOSEimproves semantic clarity in test cases while preserving all test logic.Also applies to: 47-47, 65-65
core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (1)
143-143: LGTM! Enhanced JSONPath support for arrays and primitives.The change from
getBodyAsStream()togetBodyAsStreamDecoded()ensures proper character encoding handling, and switching fromMap.classtoObject.classenables JSONPath to evaluate arrays and primitive JSON values (not just objects). This aligns with the new test coverage for arrays and numbers in JsonpathExchangeExpressionTest.java.core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java (1)
35-35: LGTM! Clean refactoring to static factory method.The switch from
ExchangeExpression.newInstance()to the statically importedexpression()method is consistent with the codebase-wide refactoring.Also applies to: 58-58
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java (1)
27-27: LGTM! Consistent with the factory method refactoring.Also applies to: 38-38
core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
27-27: LGTM! Factory method updated consistently.Also applies to: 99-99
core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java (1)
33-33: LGTM! Factory method refactoring applied correctly.Also applies to: 97-97
core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java (1)
29-29: LGTM! Test utilities updated to use the new factory method.Also applies to: 69-69, 73-73, 77-77
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyKeyComplexMatchTest.java (1)
32-32: LGTM! Test cases refactored consistently.Also applies to: 58-58, 72-72, 76-76
core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (1)
28-28: LGTM! Excellent test coverage for the enhanced JSONPath functionality.The new test cases for arrays (lines 124-128) and primitive numbers (lines 130-134) validate the switch to
Object.classin JsonpathExchangeExpression.java, ensuring JSONPath can now handle non-object JSON values correctly. The factory method refactoring is also applied consistently.Also applies to: 120-134, 137-137
core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetPropertyInterceptorSpELTest.java (2)
17-17: LGTM!The new imports support the
body()test method. The wildcard imports are appropriate for test code.Also applies to: 24-25
86-94: LGTM! Body interpolation test correctly validates SpEL decoded body access.The test properly verifies that the
${body}placeholder is interpolated with the decoded request body content. The test structure follows the established AAA pattern and is consistent with other tests in the class.core/src/test/java/com/predic8/membrane/core/interceptor/flow/invocation/FlowTestInterceptors.java (1)
54-59: LGTM! Rename from CHOICE to CHOOSE is complete and verified.The rename is successful across the codebase. The CHOOSE method definition exists at line 54 and is actively used in multiple test cases (ChooseInterceptorTest.java at lines 29, 47, 65). No remaining references to the old CHOICE method name were found. The change improves consistency with other helper methods by using verb forms.
core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java (1)
51-58: Delete the verification request; migration is complete and correctly implemented.The script results show zero calls to
ExchangeExpression.newInstance(), confirming the rename toexpression()was fully migrated. The two matches forTemplateExchangeExpression.newInstance()are unrelated—this is a separate class with an independent factory method for template parsing, not part of the refactoring under review.Likely an incorrect or invalid review comment.
…path' into scripting-body-access-array-jsonpath
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/flow/CallInterceptor.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/flow/CallInterceptorTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/flow/CallInterceptor.java (1)
90-94: Excellent fix for binary content preservation.The past review correctly identified that using
getBodyAsStringDecoded().getBytes()would corrupt binary data and cause charset mismatches. The current implementation using.getBody().getContent()properly addresses this concern by preserving the raw content without a decode/encode cycle. The added comment clearly explains that decoding happens later when the content is accessed viagetBodyAsStringDecoded().core/src/test/java/com/predic8/membrane/core/interceptor/flow/CallInterceptorTest.java (1)
41-42: LGTM! Test correctly validates the new header filtering behavior.The test updates accurately reflect the implementation changes where
TRANSFER_ENCODINGandCONTENT_ENCODINGare now preserved when copying response headers to the request, whileSERVERcontinues to be filtered out. The assertions are correct and the test clearly distinguishes between preserved and filtered headers.Also applies to: 48-54
…le transfer encoding
…path' into scripting-body-access-array-jsonpath
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java (1)
26-32: Use decoded body in converter for consistency with LazyBody.toString().The converter should use
body.getMessage().getBodyAsStringDecoded()instead of creating a String from raw bytes without charset. This aligns withLazyBody.toString()behavior and matches the pattern used throughout the codebase for body-to-string conversions.Update line 28:
return body.getMessage().getBodyAsStringDecoded();
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java (1)
16-16: Consider using explicit import instead of wildcard.While this works correctly, explicit imports (e.g.,
import com.predic8.membrane.core.lang.LazyBody;) improve code clarity and prevent potential naming conflicts.-import com.predic8.membrane.core.lang.*; +import com.predic8.membrane.core.lang.LazyBody;core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java (1)
21-21: Consider using explicit import instead of wildcard.Replace the wildcard import with an explicit import for better clarity.
-import com.predic8.membrane.core.lang.*; +import com.predic8.membrane.core.lang.LazyBody;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/lang/LazyBody.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java(4 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/lang/LazyBody.java (2)
20-36: Well-documented lazy evaluation design.The Javadoc clearly explains the purpose, and storing the message reference enables extraction of compressed bodies. Good design.
38-44: No changes needed; error handling is appropriate.The
toString()method implementation is correct. The underlyinggetBodyAsStringDecoded()method already handles exceptions by wrapping them inRuntimeException(Message.java, lines 139-144). Since LazyBody is specifically designed for SpEL expressions (per its Javadoc), allowing exceptions to propagate as RuntimeExceptions is the appropriate behavior—callers needing specific error handling can wrap the expression evaluation itself.core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java (1)
43-43: Consistent refactoring to LazyBody.The field declaration, initialization, and getter are all properly updated to use
LazyBody. The integration with the SpEL evaluation context is correct.Also applies to: 74-74, 178-178
|
/ok-to-test |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests